Skip to content

feat(bigtable): support x-goog-user-project#8324

Merged
coryan merged 5 commits into
googleapis:mainfrom
coryan:feat-bigtable-support-x-goog-user-project
Feb 10, 2022
Merged

feat(bigtable): support x-goog-user-project#8324
coryan merged 5 commits into
googleapis:mainfrom
coryan:feat-bigtable-support-x-goog-user-project

Conversation

@coryan
Copy link
Copy Markdown
Contributor

@coryan coryan commented Feb 9, 2022

Part of the work for #8284


This change is Reviewable

@product-auto-label product-auto-label Bot added the api: bigtable Issues related to the Bigtable API. label Feb 9, 2022
@google-cloud-cpp-bot
Copy link
Copy Markdown
Contributor

Google Cloud Build Logs
For commit: 14848cc77efa49f43636c09c029dcc0f4a73e999

ℹ️ NOTE: Kokoro logs are linked from "Details" below.

@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 9, 2022

Codecov Report

Merging #8324 (f53f8bd) into main (0e23992) will decrease coverage by 0.00%.
The diff coverage is 92.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #8324      +/-   ##
==========================================
- Coverage   94.97%   94.96%   -0.01%     
==========================================
  Files        1345     1345              
  Lines      119817   119871      +54     
==========================================
+ Hits       113793   113839      +46     
- Misses       6024     6032       +8     
Impacted Files Coverage Δ
google/cloud/bigtable/data_client.cc 83.54% <80.95%> (-0.94%) ⬇️
google/cloud/bigtable/internal/defaults.cc 100.00% <100.00%> (ø)
google/cloud/bigtable/internal/defaults_test.cc 100.00% <100.00%> (ø)
...le/cloud/storage/internal/curl_download_request.cc 89.04% <0.00%> (-1.07%) ⬇️
...cloud/pubsub/internal/subscription_session_test.cc 98.00% <0.00%> (-0.75%) ⬇️
google/cloud/completion_queue_test.cc 96.95% <0.00%> (-0.20%) ⬇️
google/cloud/pubsub/samples/samples.cc 92.10% <0.00%> (+0.07%) ⬆️
google/cloud/storage/internal/curl_handle.cc 73.03% <0.00%> (+0.56%) ⬆️
...ud/spanner/integration_tests/client_stress_test.cc 86.18% <0.00%> (+0.65%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0e23992...f53f8bd. Read the comment docs.

Copy link
Copy Markdown
Member

@dbolduc dbolduc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 3 files at r1, all commit messages.
Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @coryan and @dbolduc)


google/cloud/bigtable/data_client.cc, line 116 at r1 (raw file):

  ReadRows(grpc::ClientContext* context,
           btproto::ReadRowsRequest const& request) override {
    return impl_.Stub()->ReadRows(context, request);

I think we need one here.


google/cloud/bigtable/internal/defaults_test.cc, line 164 at r1 (raw file):

  env = ScopedEnvironment("GOOGLE_CLOUD_CPP_USER_PROJECT", "env-project");
  options = Options{}.set<UserProjectOption>("env-project");

s/env-project/test-project/

to make sure the user supplied options gets overridden by the env var

@google-cloud-cpp-bot
Copy link
Copy Markdown
Contributor

Google Cloud Build Logs
For commit: 4d138ad7d7d5c44652fc7f760e4da9f3a7fe3ea1

ℹ️ NOTE: Kokoro logs are linked from "Details" below.

Copy link
Copy Markdown
Member

@dbolduc dbolduc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this


TEST(OptionsTest, InstanceAdminUserProjectOption) {
auto env = ScopedEnvironment("GOOGLE_CLOUD_CPP_USER_PROJECT", absl::nullopt);
auto options = DefaultInstanceAdminOptions(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be fine with there just being one test, which calls DefaultOptions( ... ), instead of a test for each service... but your call.

@google-cloud-cpp-bot
Copy link
Copy Markdown
Contributor

Google Cloud Build Logs
For commit: fc88475983a8f0d5d32332a4c597e11d8a0894cf

ℹ️ NOTE: Kokoro logs are linked from "Details" below.

@dbolduc
Copy link
Copy Markdown
Member

dbolduc commented Feb 10, 2022

Can you please undo the changes to TableAdminClient? None of those methods will exist in the next release

Copy link
Copy Markdown
Contributor Author

@coryan coryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 of 4 files reviewed, 3 unresolved discussions (waiting on @coryan and @dbolduc)


google/cloud/bigtable/data_client.cc, line 116 at r1 (raw file):

Previously, dbolduc (Darren Bolduc) wrote…

I think we need one here.

Done.


google/cloud/bigtable/internal/defaults_test.cc, line 164 at r1 (raw file):

Previously, dbolduc (Darren Bolduc) wrote…

s/env-project/test-project/

to make sure the user supplied options gets overridden by the env var

Fixed. Also fixed the test to be useful. And also added tests for admin and instance admin.

@coryan
Copy link
Copy Markdown
Contributor Author

coryan commented Feb 10, 2022

Can you please undo the changes to TableAdminClient? None of those methods will exist in the next release

But they exist now? People can pull code from main. And you may win the lottery and not complete the work.

@google-cloud-cpp-bot
Copy link
Copy Markdown
Contributor

Google Cloud Build Logs
For commit: f53f8bda474800f720a6969c44e7097299decbe0

ℹ️ NOTE: Kokoro logs are linked from "Details" below.

@dbolduc
Copy link
Copy Markdown
Member

dbolduc commented Feb 10, 2022

Can you please undo the changes to TableAdminClient? None of those methods will exist in the next release

But they exist now? People can pull code from main. And you may win the lottery and not complete the work.

Ok fine. When the time comes, I will delete this with pleasure.

@coryan coryan marked this pull request as ready for review February 10, 2022 12:14
@coryan coryan requested a review from a team February 10, 2022 12:14
@coryan coryan merged commit 77af2b6 into googleapis:main Feb 10, 2022
@coryan coryan deleted the feat-bigtable-support-x-goog-user-project branch February 10, 2022 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: bigtable Issues related to the Bigtable API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants